-
Notifications
You must be signed in to change notification settings - Fork 21
lib: add ble_radio_notification and sample #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
74df57c
to
2f5eb93
Compare
You can find the documentation preview for this PR here. |
2f5eb93
to
e110194
Compare
e110194
to
afdbe0a
Compare
8c2a381
to
974e8c4
Compare
974e8c4
to
15c45c2
Compare
CODEOWNERS
Outdated
/tests/lib/bm_zms/ @nrfconnect/ncs-bm @rghaddab | ||
/tests/lib/ble_qwr/ @nrfconnect/ncs-bm | ||
/tests/lib/ble_racp/ @nrfconnect/ncs-bm | ||
/tests/lib/ble_radio_notif/ @nrfconnect/ncs-bm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alignment is off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.
#define STATIC static | ||
#endif | ||
|
||
LOG_MODULE_REGISTER(ble_radio_ntf, CONFIG_BLE_RADIO_NTF_LOG_LEVEL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOG_MODULE_REGISTER(ble_radio_ntf, CONFIG_BLE_RADIO_NTF_LOG_LEVEL); | |
LOG_MODULE_REGISTER(ble_radio_notification, CONFIG_BLE_RADIO_NOTIFICATION_LOG_LEVEL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
lib/ble_radio_notification/Kconfig
Outdated
IRQ Priority level 0 and 4 are reserved by the SoftDevice. | ||
|
||
|
||
module=BLE_RADIO_NTF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module=BLE_RADIO_NTF | |
module=BLE_RADIO_NOTIFICATION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.
range 50 5500 | ||
default 800 | ||
|
||
module=BLE_RADIO_NTF_SAMPLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix names as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
void setUp(void) | ||
{ | ||
} | ||
void tearDown(void) | ||
{ | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void setUp(void) | |
{ | |
} | |
void tearDown(void) | |
{ | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
15c45c2
to
cf7859c
Compare
/* Application event handler for handling Radio Notification events. */ | ||
static ble_radio_notification_evt_handler_t evt_handler; | ||
|
||
STATIC void radio_notification_isr(const void *arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we using STATIC
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Se definition above. It is to be able to call the event handler from the unit test.
} | ||
} | ||
|
||
uint32_t ble_radio_notification_init(uint32_t distance, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not when we are returning nrf_error.
type = NRF_RADIO_NOTIFICATION_TYPE_INT_ON_INACTIVE; | ||
#endif | ||
|
||
evt_handler = _evt_handler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the user be able to unset the callback? If not, we could check against NULL
here, and avoid doing at every interrupt before invoking the callback. Otherwise, If it should be possible to unset the callback, we could provide an explicit de-initialization function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will move the check for evt_handler to the initialization. I don't think having a callback-unset would provide much value, unless that is also disabling the IRQ. Though, in that case, if disabling while the radio is active, the radio active state might be messed up.
#if defined CONFIG_BLE_RADIO_NOTIFICATION_ON_BOTH | ||
radio_active = !radio_active; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would use the macro IS_ENABLED()
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do that anywhere else, only for runtime if's.
cf7859c
to
a448920
Compare
a448920
to
45f9ef4
Compare
45f9ef4
to
93ae69f
Compare
@nordicjm Please have another look. |
:local: | ||
:depth: 2 | ||
|
||
The Bluetooth Low Energy® Radio Notification library allows the user to subscribe to the Radio Notification signal to receive notification prior to and after radio events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Bluetooth Low Energy® Radio Notification library allows the user to subscribe to the Radio Notification signal to receive notification prior to and after radio events. | |
The Bluetooth Low Energy® Radio Notification library allows the user to subscribe to the Radio Notification signal to receive notifications before and after radio events. |
Overview | ||
******** | ||
|
||
The library allows the user to register a handler to receive the radio notifications and configure the distance in microseconds between the active Radio Notification signal and the radio event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library allows the user to register a handler to receive the radio notifications and configure the distance in microseconds between the active Radio Notification signal and the radio event. | |
The library allows the user to register a handler to receive radio notifications and configure the distance in microseconds between the active Radio Notification signal and the radio event. |
|
||
The library uses the ``SWI02`` IRQ. | ||
Use the :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_IRQ_PRIO` Kconfig option to set the IRQ priority. | ||
The library can be configured to receive notifications before the radio is active, after the radio is active or both before and after. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library can be configured to receive notifications before the radio is active, after the radio is active or both before and after. | |
The library can be configured to receive notifications before the radio is active, after the radio is active, or both before and after. |
The library uses the ``SWI02`` IRQ. | ||
Use the :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_IRQ_PRIO` Kconfig option to set the IRQ priority. | ||
The library can be configured to receive notifications before the radio is active, after the radio is active or both before and after. | ||
This can be chosen by setting the :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_ACTIVE`, :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_INACTIVE` or :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_BOTH` Kconfig option, respectively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be chosen by setting the :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_ACTIVE`, :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_INACTIVE` or :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_BOTH` Kconfig option, respectively. | |
This can be chosen by setting the :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_ACTIVE`, the :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_INACTIVE`, or the :kconfig:option:`CONFIG_BLE_RADIO_NOTIFICATION_ON_BOTH` Kconfig option, respectively. |
| Header file: :file:`include/ble_radio_notification.h` | ||
| Source files: :file:`lib/ble_radio_notification/` | ||
.. doxygengroup:: ble_radio_notification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. doxygengroup:: ble_radio_notification | |
:ref:`Bluetooth LE Radio Notification library API reference <api_ble_radio_notif>` |
include/ble_radio_notification.h
Outdated
extern "C" { | ||
#endif | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra newline.
include/ble_radio_notification.h
Outdated
* @brief Function for initializing the Radio Notification module. | ||
* | ||
* @param[in] distance Distance between the ACTIVE notification signal and start of radio event. | ||
* @param[in] evt_handler Handler to be executed when a radio notification event has been received. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param[in] evt_handler Handler to be executed when a radio notification event has been received. | |
* @param[in] evt_handler Handler to be called when a radio notification event has been received. |
include/ble_radio_notification.h
Outdated
* @param[in] distance Distance between the ACTIVE notification signal and start of radio event. | ||
* @param[in] evt_handler Handler to be executed when a radio notification event has been received. | ||
* | ||
* @return NRF_SUCCESS on successful initialization, otherwise an error code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not be more specific with the error codes returned here?
lib/ble_radio_notification/Kconfig
Outdated
default BLE_RADIO_NOTIFICATION_ON_BOTH | ||
|
||
config BLE_RADIO_NOTIFICATION_ON_ACTIVE | ||
bool "on active" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool "on active" | |
bool "On active" |
lib/ble_radio_notification/Kconfig
Outdated
bool "on active" | ||
|
||
config BLE_RADIO_NOTIFICATION_ON_INACTIVE | ||
bool "on inactive" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool "on inactive" | |
bool "On inactive" |
lib/ble_radio_notification/Kconfig
Outdated
bool "on inactive" | ||
|
||
config BLE_RADIO_NOTIFICATION_ON_BOTH | ||
bool "on both active and inactive" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool "on both active and inactive" | |
bool "On both active and inactive" |
help | ||
IRQ Priority level 0 and 4 are reserved by the SoftDevice. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra newline.
help | ||
IRQ Priority level 0 and 4 are reserved by the SoftDevice. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/ble_radio_notification/Kconfig
Outdated
|
||
|
||
module=BLE_RADIO_NOTIFICATION | ||
module-dep=LOG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module-dep=LOG |
this variable does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used everywhere, including https://github.com/nrfconnect/sdk-nrf-bm/blob/main/samples/boot/mcuboot_recovery_entry/Kconfig#L10. If this should be removed it should be done repo-wide as a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes cleanup in different PR but this PR updated to not add them in the new files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(removed for the new files in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And opened #405 for cleanup.
} | ||
|
||
uint32_t ble_radio_notification_init(uint32_t distance, | ||
ble_radio_notification_evt_handler_t _evt_handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not start variables with an underscore, this naming is reserved for compiler internal variables
return NRF_ERROR_NULL; | ||
} | ||
|
||
#if defined(CONFIG_BLE_RADIO_NOTIFICATION_ON_ACTIVE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
un-indent line
default 800 | ||
|
||
module=BLE_RADIO_NOTIFICATION_SAMPLE | ||
module-dep=LOG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module-dep=LOG |
remove from whole PR
extern void radio_notification_isr(const void *arg); | ||
bool radio_active; | ||
|
||
static void ble_radio_notification_evt_handler(bool _radio_active) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
|
||
void on_conn_params_evt(const struct ble_conn_params_evt *evt) | ||
{ | ||
int err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int err; | |
uint32_t err; |
|
||
LOG_INF("SoftDevice enabled"); | ||
|
||
err = ble_radio_notification_init(CONFIG_BLE_RADIO_NOTIFICATION_DISTANCE_US, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns NRF errors. Need to address variable type and log message.
Overview | ||
******** | ||
|
||
The sample initialize the radio notification library and register a handler to trigger both when the radio will be enabled and disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample initialize the radio notification library and register a handler to trigger both when the radio will be enabled and disabled. | |
The sample initializes the radio notification library and registers a handler to trigger both when the radio will be enabled and disabled. |
******** | ||
|
||
The sample initialize the radio notification library and register a handler to trigger both when the radio will be enabled and disabled. | ||
The handler toggle the LED0 depending on the radio state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handler toggle the LED0 depending on the radio state. | |
The handler toggles the LED0 depending on the radio state. |
93ae69f
to
dff7819
Compare
# | ||
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
# | ||
menuconfig BLE_RADIO_NOTIFICATION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that this is not expressing the dependency on SoftDevice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Note that this applies for most of the bluetooth libraries. So we should clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eivindj-nordic What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not have this dependency for many of the other ble libraries. See connection parameters etc.
137e477
to
a34885f
Compare
fe2b67f
to
ba0b865
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, but please address comments.
ba0b865
to
bad6849
Compare
@nordicjm Please have another look. |
bad6849
to
aa43dd2
Compare
include/ble_radio_notification.h
Outdated
|
||
#include <stdint.h> | ||
#include <stdbool.h> | ||
#include <nrf_soc.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this is not needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to c-file.
CONFIG_LOG_BACKEND_BM_UARTE=y | ||
|
||
CONFIG_SOFTDEVICE=y | ||
CONFIG_NRF_SDH=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just CONFIG_SOFTDEVICE
should work now
CONFIG_BLE_ADV_FAST_ADVERTISING_INTERVAL=400 | ||
CONFIG_BLE_ADV_SLOW_ADVERTISING_INTERVAL=1600 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to override defaults here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly no, though it makes it more observable by looking at the LED. With default intervals it blinks quite fast regardless of the advertising mode.
NRF54L15_XXAA | ||
CONFIG_BLE_RADIO_NOTIFICATION_ON_BOTH=1 | ||
CONFIG_BLE_RADIO_NOTIFICATION_IRQ_PRIO=5 | ||
SWI02_IRQn=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhm why SWI02_IRQn=0
(equal zero?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just needed to be defined to something, updated to 30 as is what it is defined to for nRF54l15.
142bcf3
to
8cc6735
Compare
Add ble_radio_notification library and sample. Signed-off-by: Eivind Jølsgard <[email protected]>
8cc6735
to
e6dd6fd
Compare
Add ble_radio_notification library and sample.